Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update code for loadtests #764

Merged
merged 7 commits into from
Jul 12, 2024
Merged

Update code for loadtests #764

merged 7 commits into from
Jul 12, 2024

Conversation

Nico-Sanchez
Copy link
Collaborator

@Nico-Sanchez Nico-Sanchez commented Jul 10, 2024

Motivation

Loadtests weren't working with current code, several changes made in arena broke the loadtests app.

Summary of changes

How to test it?

Run some loadtests and see if it works. Remember to set the new env var.
You can follow loadtest's README to do so.

Checklist

  • Tested the changes locally.
  • Reviewed the changes on GitHub, line by line.
  • This change requires new documentation.
    • Documentation has been added/updated.

Copy link
Contributor

@tvillegas98 tvillegas98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't seem to work straightforwardly yet. Here is the list of things I have to do to use it.

  1. There's a function named ArenaLoadTest.Utils.get_server_ip/1) that might not be longer needed (because the env var TARGET_SERVER now uses a public URL. This caused the load test to crash because the pattern matching doesn't catch this
  2. In the load test server, I had to set the env var OVERRRIDE_JWT to true (I understand that this is going to be solved after we merged this PR)

I think we only need to solve 1)

@tvillegas98
Copy link
Contributor

As a heads up too, we might have to update the current load_testing.md documentation

@Nico-Sanchez
Copy link
Collaborator Author

As a heads up too, we might have to update the current load_testing.md documentation

Hmm I believe that was the old loadtest docs, we moved it when we split the repos.
The docs I maintain is the README. We could delete it or leave it for further improvements (it has useful info like the tooling section).

@Nico-Sanchez
Copy link
Collaborator Author

It doesn't seem to work straightforwardly yet. Here is the list of things I have to do to use it.

  1. There's a function named ArenaLoadTest.Utils.get_server_ip/1) that might not be longer needed (because the env var TARGET_SERVER now uses a public URL. This caused the load test to crash because the pattern matching doesn't catch this
  2. In the load test server, I had to set the env var OVERRRIDE_JWT to true (I understand that this is going to be solved after we merged this PR)

I think we only need to solve 1)

Done both!

@tvillegas98
Copy link
Contributor

As a heads up too, we might have to update the current load_testing.md documentation

Hmm I believe that was the old loadtest docs, we moved it when we split the repos. The docs I maintain is the README. We could delete it or leave it for further improvements (it has useful info like the tooling section).

Thanks for the clarification! I thought that the information would be in that markdown. Maybe we can merge both markdowns 😄

Copy link
Contributor

@tvillegas98 tvillegas98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work fixing load test! I only had to run the spawn players function to execute it!

Screen.Recording.2024-07-11.at.18.27.29.mov

Copy link
Contributor

@agustinesco agustinesco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! tested deploying both servers and the load tests runned like a charm

@agustinesco agustinesco merged commit d971adc into main Jul 12, 2024
7 checks passed
@agustinesco agustinesco deleted the update-code-for-loadtests branch July 12, 2024 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants